-
Notifications
You must be signed in to change notification settings - Fork 22
feat(opentelemetry): create otel instrumentation for typed-express-router #1044
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: beta
Are you sure you want to change the base?
feat(opentelemetry): create otel instrumentation for typed-express-router #1044
Conversation
9c177cd
to
0bff27e
Compare
packages/opentelemetry-instrumentation-typed-express-router/test/instrumentation.test.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-instrumentation-typed-express-router/test/instrumentation.test.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-instrumentation-typed-express-router/src/version.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-instrumentation-typed-express-router/src/utils.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts
Fixed
Show fixed
Hide fixed
43daf20
to
3781856
Compare
packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-express/src/terinstrumentation.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-express/src/terinstrumentation.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-express/src/terinstrumentation.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-express/src/terinstrumentation.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-express/src/terinstrumentation.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-express/src/terinstrumentation.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-express/src/terinstrumentation.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-express/src/terinstrumentation.ts
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-express/test/fixtures/use-express.mjs
Fixed
Show fixed
Hide fixed
packages/opentelemetry-instrumentation-typed-express-router/src/instrumentation.ts
Fixed
Show fixed
Hide fixed
6cd264b
to
11fc7f0
Compare
f804fb4
to
0158ca3
Compare
a224511
to
ab8a123
Compare
183d5f4
to
3cfad4e
Compare
…uter This commit implements `opentelemetry` instrumentation for the `decode` and `sendEncoded` calls in `wrapRouter`. Changes: - `typed-express-wrapper` - `@opentelemetry/api@^1.0.0` added as an optional peer dependency - `@opentelemetry/[email protected]` , `@opentelemetry/[email protected]`, and `@opentelemetry/[email protected]` added as dev dependencies - `wrapRouter` modified to create decode and encode spans if `@opentelemetry/api` is installed - if `@opentelemetry/api` is not installed, spans are not created - `onDecodeError` option removed. Please use `decodeErrorFormatter` and `getDecodeErrorStatusCode` - `decodeErrorFormatter` takes in an array of `ValidationError`s and a `WrappedRequest`, returning a `Json` object. - `getDecodeErrorStatusCode` takes in an array of `ValidationError`s and a `WrappedRequest`, returning a number. - `onEncodeError` option removed. Please use `encodeErrorFormatter` and `getEncodeErrorStatusCode` - `encodeErrorFormatter` takes in an error and a `WrappedRequest`, returning a `Json` object. - `getEncodeErrorStatusCode` takes in an error and a `WrappedRequest`, returning a number. - `typed-express-router` now handles the sending of the http response when there is a decode or encode error - `express-wrapper` - `routerForApiSpec` modified to pass in new parameters `decodeErrorFormatter`, `getDecodeErrorStatusCode`, `encodeErrorFormatter`, and `getEncodeErrorStatusCode` - These new parameters can also be customized by modifying the props passed into the function (see `CreateRouterProps`) BREAKING CHANGE: `onDecodeError` and `onEncodeError` have been removed. Please use `decodeErrorFormatter`, `getDecodeErrorStatusCode`, `encodeErrorFormatter`, and `getEncodeErrorStatusCode`
3cfad4e
to
4248ffd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently this text box can't be empty.
}; | ||
|
||
export const onEncodeError: OnEncodeErrorFn = (err, _req, res) => { | ||
export const defaultEncodeErrorFormatter: EncodeErrorFormatterFn = (_err, _req) => { | ||
return {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Does this change the behavior of express-wrapper so it sends a JSON body of {}
, where previously there was no body?
_req, | ||
) => 400; | ||
|
||
export const defaultEncodeErrorFormatter: EncodeErrorFormatterFn = () => ({}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idem
onEncodeError: (_err, _req, res) => { | ||
res.status(500).json('Custom encode error').end(); | ||
encodeErrorFormatter: (_err, _req) => { | ||
return 'Custom encode error'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: Look at all that data instead of state! 😍
import { createRouter } from '../src'; | ||
import assert from 'node:assert'; | ||
import { ApiTsAttributes } from '../src/telemetry'; | ||
import test, { beforeEach, afterEach } from 'node:test'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quibble: please separate imports into the following sections, with sections separated by a single newline:
- Native node imports (begin with
node:
) - Imports from a named package
- Relative imports
Note: Not blocking change, can be done in a follow-up PR.
body?: any, | ||
): Promise<{ statusCode: number; data: string }> => { | ||
return new Promise((resolve, reject) => { | ||
const url = `http://localhost:${(server.address() as any).port}${path}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: LSP says server.address()
is of type string | AddressInfo | null
-- how do you always know it is of type AddressInfo
here? I wonder if there's a way to clean up the type cast.
const GetHello = httpRoute({ | ||
path: '/hello/{id}', | ||
method: 'GET', | ||
request: httpRequest({ | ||
params: { | ||
id: t.string, | ||
}, | ||
}), | ||
response: { | ||
200: t.type({ | ||
id: t.string, | ||
}), | ||
}, | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quibble: Please use the conventional order of GET, POST, PUT, DELETE, PATCH, by moving this GET operation above the PUT operation declared for the same path on line 58.
api-ts/packages/typed-express-router/src/index.ts
Lines 190 to 194 in a203328
get: makeAddRoute('get'), | |
post: makeAddRoute('post'), | |
put: makeAddRoute('put'), | |
delete: makeAddRoute('delete'), | |
patch: makeAddRoute('patch'), |
Note: Not a blocking change, can be done in a follow-up PR.
put: PutHello, | ||
get: GetHello, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quibble: For the same reason as above, please use the order of GET, POST, PUT, DELETE, PATCH
Note: Not a blocking change, can be done in a follow-up PR.
decodeSpan?.attributes?.[ApiTsAttributes.API_TS_PATH], | ||
'/hello/{id}', | ||
); | ||
assert.strictEqual(decodeSpan?.attributes?.[ApiTsAttributes.API_TS_METHOD], 'GET'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shorthand now as we are on a call: please update ordering to match the order keys are introduced in the object definition
Note: Not a blocking change, can be done in a follow-up PR.
|
||
// TODO: POST | ||
// TODO: DELETE | ||
// TODO: PATCH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noting
// Find encode span | ||
const decodeSpan = spans.find( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noting irregular use of encode/decode
Ticket: DX-1473
This PR implements
opentelemetry
instrumentation for thedecode
andsendEncoded
calls inwrapRouter
.Changes:
typed-express-wrapper
@opentelemetry/api@^1.0.0
added as an optional peer dependency@opentelemetry/[email protected]
,@opentelemetry/[email protected]
, and@opentelemetry/[email protected]
added as dev dependencieswrapRouter
modified to create decode and encode spans if@opentelemetry/api
is installed@opentelemetry/api
is not installed, spans are not createdonDecodeError
option removed. Please usedecodeErrorFormatter
andgetDecodeErrorStatusCode
decodeErrorFormatter
takes in an array ofValidationError
s and aWrappedRequest
, returning aJson
object.getDecodeErrorStatusCode
takes in an array ofValidationError
s and aWrappedRequest
, returning a number.onEncodeError
option removed. Please useencodeErrorFormatter
andgetEncodeErrorStatusCode
encodeErrorFormatter
takes in an error and aWrappedRequest
, returning aJson
object.getEncodeErrorStatusCode
takes in an error and aWrappedRequest
, returning a number.typed-express-router
now handles the sending of the http response when there is a decode or encode errorexpress-wrapper
routerForApiSpec
modified to pass in new parametersdecodeErrorFormatter
,getDecodeErrorStatusCode
,encodeErrorFormatter
, andgetEncodeErrorStatusCode
CreateRouterProps
)BREAKING CHANGE:
onDecodeError
andonEncodeError
have been removed. Please usedecodeErrorFormatter
,getDecodeErrorStatusCode
,encodeErrorFormatter
, andgetEncodeErrorStatusCode